gh-149101: Implement PEP 788#149116
Conversation
Documentation build overview
25 files changed ·
|
encukou
left a comment
There was a problem hiding this comment.
Thanks for adding these!
I'll send notes for Doc/ now; code review coming up.
| Currently, this function will deallocate *view*, but this may change in | ||
| the future. | ||
|
|
There was a problem hiding this comment.
Is this relevant for the user? Allocation looks like an implementation detail here.
There was a problem hiding this comment.
Not sure -- I added it thinking about debugging (e.g., if you're trying to find a missing PyInterpreterGuard_Close call, you can just look for unfreed PyInterpreterGuard * pointers).
There was a problem hiding this comment.
If you're in a debugger already, I don't think you need docs to notice this implementation detail.
| The interpreter referenced by *view* will be implicitly guarded. The | ||
| guard will be released upon the corresponding :c:func:`PyThreadState_Release` | ||
| call. | ||
|
|
||
| On success, this function will return the thread state that was previously attached. | ||
| If no thread state was previously attached, this returns a non-``NULL`` sentinel | ||
| value. The behavior of whether this function creates a thread state is | ||
| equivalent to that of :c:func:`PyThreadState_Ensure`. | ||
|
|
||
| To visualize, function is roughly equivalent to the following: | ||
|
|
||
| .. code-block:: c | ||
|
|
||
| PyThreadState * | ||
| PyThreadState_EnsureFromView(PyInterpreterView *view) | ||
| { | ||
| assert(view != NULL); | ||
| PyInterpreterGuard *guard = PyInterpreterGuard_FromView(view); | ||
| if (guard == NULL) { | ||
| return NULL; | ||
| } | ||
|
|
||
| PyThreadState *tstate = PyThreadState_Ensure(guard); | ||
| if (tstate == NULL) { | ||
| PyInterpreterGuard_Close(guard); | ||
| return NULL; | ||
| } | ||
|
|
||
| if (tstate->guard == NULL) { | ||
| tstate->guard = guard; | ||
| } else { | ||
| PyInterpreterGuard_Close(guard); | ||
| } | ||
|
|
||
| return tstate; | ||
| } |
There was a problem hiding this comment.
Consider not repeating complex information; it can leave the reader wondering whether it actually is the same.
Only repeat the warning.
| The interpreter referenced by *view* will be implicitly guarded. The | |
| guard will be released upon the corresponding :c:func:`PyThreadState_Release` | |
| call. | |
| On success, this function will return the thread state that was previously attached. | |
| If no thread state was previously attached, this returns a non-``NULL`` sentinel | |
| value. The behavior of whether this function creates a thread state is | |
| equivalent to that of :c:func:`PyThreadState_Ensure`. | |
| To visualize, function is roughly equivalent to the following: | |
| .. code-block:: c | |
| PyThreadState * | |
| PyThreadState_EnsureFromView(PyInterpreterView *view) | |
| { | |
| assert(view != NULL); | |
| PyInterpreterGuard *guard = PyInterpreterGuard_FromView(view); | |
| if (guard == NULL) { | |
| return NULL; | |
| } | |
| PyThreadState *tstate = PyThreadState_Ensure(guard); | |
| if (tstate == NULL) { | |
| PyInterpreterGuard_Close(guard); | |
| return NULL; | |
| } | |
| if (tstate->guard == NULL) { | |
| tstate->guard = guard; | |
| } else { | |
| PyInterpreterGuard_Close(guard); | |
| } | |
| return tstate; | |
| } | |
| On success, the interpreter referenced by *view* will be implicitly guarded; | |
| the guard will be released upon the corresponding :c:func:`PyThreadState_Release` | |
| call. | |
| Otherwise, the behavior and return value are the same as for | |
| :c:func:`PyThreadState_Ensure`. | |
| Note that the returned pointer is not necessarily a valid | |
| :c:type:`!PyThreadState` pointer. |
|
|
||
| This function will decrement an internal counter on the attached thread state. If | ||
| this counter ever reaches below zero, this function emits a fatal error (via | ||
| :c:func:`Py_FatalError`). | ||
|
|
||
| If the attached thread state is owned by ``PyThreadState_Ensure``, then the | ||
| attached thread state will be deallocated and deleted upon the internal counter | ||
| reaching zero. Otherwise, nothing happens when the counter reaches zero. | ||
|
|
||
| If *tstate* is non-``NULL``, it will be attached upon returning. | ||
| If *tstate* indicates that no prior thread state was attached, there will be | ||
| no attached thread state upon returning. | ||
|
|
||
| To visualize, this function is roughly equivalent to the following: | ||
|
|
||
| .. code-block:: c | ||
|
|
||
| void | ||
| PyThreadState_Release(PyThreadState *old_tstate) | ||
| { | ||
| PyThreadState *current_tstate = PyThreadState_Get(); | ||
| assert(old_tstate != NULL); | ||
| assert(current_tstate != NULL); | ||
| assert(current_tstate->ensure_counter > 0); | ||
| if (--current_tstate->ensure_counter > 0) { | ||
| // There are remaining PyThreadState_Ensure() calls | ||
| // for this thread state. | ||
| return; | ||
| } | ||
|
|
||
| assert(current_tstate->ensure_counter == 0); | ||
| if (old_tstate == NO_TSTATE_SENTINEL) { | ||
| // No thread state was attached prior the PyThreadState_Ensure() | ||
| // call. So, we can just destroy the current thread state and return. | ||
| assert(current_tstate->owned_by_pythreadstate_ensure); | ||
| PyThreadState_Clear(current_tstate); | ||
| PyThreadState_DeleteCurrent(); | ||
| return; | ||
| } | ||
|
|
||
| if (tstate->guard != NULL) { | ||
| PyInterpreterGuard_Close(tstate->guard); | ||
| return; | ||
| } | ||
|
|
||
| if (tstate->owned_by_pythreadstate_ensure) { | ||
| // The attached thread state was created by the initial PyThreadState_Ensure() | ||
| // call. It's our job to destroy it. | ||
| PyThreadState_Clear(current_tstate); | ||
| PyThreadState_DeleteCurrent(); | ||
| } | ||
|
|
||
| PyThreadState_Swap(old_tstate); | ||
| } |
There was a problem hiding this comment.
Again, leave implementation details to the implementation -- and the PEP :)
| This function will decrement an internal counter on the attached thread state. If | |
| this counter ever reaches below zero, this function emits a fatal error (via | |
| :c:func:`Py_FatalError`). | |
| If the attached thread state is owned by ``PyThreadState_Ensure``, then the | |
| attached thread state will be deallocated and deleted upon the internal counter | |
| reaching zero. Otherwise, nothing happens when the counter reaches zero. | |
| If *tstate* is non-``NULL``, it will be attached upon returning. | |
| If *tstate* indicates that no prior thread state was attached, there will be | |
| no attached thread state upon returning. | |
| To visualize, this function is roughly equivalent to the following: | |
| .. code-block:: c | |
| void | |
| PyThreadState_Release(PyThreadState *old_tstate) | |
| { | |
| PyThreadState *current_tstate = PyThreadState_Get(); | |
| assert(old_tstate != NULL); | |
| assert(current_tstate != NULL); | |
| assert(current_tstate->ensure_counter > 0); | |
| if (--current_tstate->ensure_counter > 0) { | |
| // There are remaining PyThreadState_Ensure() calls | |
| // for this thread state. | |
| return; | |
| } | |
| assert(current_tstate->ensure_counter == 0); | |
| if (old_tstate == NO_TSTATE_SENTINEL) { | |
| // No thread state was attached prior the PyThreadState_Ensure() | |
| // call. So, we can just destroy the current thread state and return. | |
| assert(current_tstate->owned_by_pythreadstate_ensure); | |
| PyThreadState_Clear(current_tstate); | |
| PyThreadState_DeleteCurrent(); | |
| return; | |
| } | |
| if (tstate->guard != NULL) { | |
| PyInterpreterGuard_Close(tstate->guard); | |
| return; | |
| } | |
| if (tstate->owned_by_pythreadstate_ensure) { | |
| // The attached thread state was created by the initial PyThreadState_Ensure() | |
| // call. It's our job to destroy it. | |
| PyThreadState_Clear(current_tstate); | |
| PyThreadState_DeleteCurrent(); | |
| } | |
| PyThreadState_Swap(old_tstate); | |
| } |
Co-authored-by: Petr Viktorin <encukou@gmail.com>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
|
🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit bc78c10 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F149116%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
|
for buildbots: The RHEL8 failures aren't relevant. Refleaks are worrying though. |
Never mind; main currently leaks (#149179). |
encukou
left a comment
There was a problem hiding this comment.
Today's part of the review
(If some comment doesn't make sense, it might be because I didn't read through everything yet. )
| [function.PyInterpreterGuard_FromView] | ||
| added = '3.15' |
There was a problem hiding this comment.
This is missing from the PEP; I assume that's an oversight. Just update the PEP when you mark it Final, and ask SC to rubber-stamp it.
| struct _PyInterpreterGuard { | ||
| PyInterpreterState *interp; | ||
| }; | ||
|
|
||
| struct _PyInterpreterView { | ||
| int64_t id; | ||
| }; |
There was a problem hiding this comment.
These don't need the underscore. (The private header is what makes the members private; if a user included the pycore header they could use PyInterpreterGuard.interp.)
IMO it's better if the struct X name matches the typedef name.
There was a problem hiding this comment.
I used the _ prefix not so users don't touch it, but so we (as maintainers) don't accidentally use it. We should use the typedef instead. (Think of this similar to how PyObject is actually struct _object, or how PyInterpreterState is actually struct _is.)
| PyThreadState *isolated_interp_tstate; | ||
| PyStatus status = Py_NewInterpreterFromConfig(&isolated_interp_tstate, &config); | ||
| if (PyStatus_Exception(status)) { | ||
| PyErr_SetString(PyExc_RuntimeError, "interpreter creation failed"); |
There was a problem hiding this comment.
Restore save_tstate here, and in test_thread_state_ensure_nested below.
There was a problem hiding this comment.
It might be a better idea to just assert(!PyStatus_Exception(status))? I avoided error checking for some of the other tests. Realistically, there's not any reason this should fail.
| for (int i = 0; i < 10; ++i) { | ||
| assert(PyThreadState_Get() == save_tstate); | ||
| PyThreadState_Release(thread_states[i]); | ||
| } |
There was a problem hiding this comment.
Shouldn't these be released in opposite order? I guess it doesn't really matter if they're the same, but if that's a detail you're testing here please add a comment.
There was a problem hiding this comment.
It's not intentional, good catch!
| /* The following places the `_PyRuntime` structure in a location that can be | ||
| * found without any external information. This is meant to ease access to the | ||
| * interpreter state for various runtime debugging tools, but is *not* an | ||
| * officially supported feature */ | ||
|
|
||
| /* Suppress deprecation warning for PyBytesObject.ob_shash */ | ||
| _Py_COMP_DIAG_PUSH | ||
| _Py_COMP_DIAG_IGNORE_DEPR_DECLS | ||
| _Py_COMP_DIAG_PUSH _Py_COMP_DIAG_IGNORE_DEPR_DECLS |
There was a problem hiding this comment.
Could you undo the unrelated changes? They make review hard, and in many cases (like this) I disagree with your style choices. If you're fixing style in existing code, make sure it actually conflicts with PEP 7 (and ideally that it also actually bothers you).
There was a problem hiding this comment.
Ugh, sorry, this wasn't intentional. I think my editor might have auto-formatted this (and I agree that it's hideous).
| // For debugging purposes, we emit a fatal error if someone | ||
| // CTRL^C'ed the process. |
There was a problem hiding this comment.
I think it's nice to have this for users. Otherwise, developers debugging a stuck process will have to pkill every time they mess something up.
| _PyEvent_Notify(&interp->finalization_guards.done); | ||
| memset(&interp->finalization_guards.done, 0, sizeof(PyEvent)); |
There was a problem hiding this comment.
Is it safe to reset an event like this while something may be waiting on it?
If it is, maybe add a _PyEvent_Reset to pycore_lock, so any future changes are more likely to take this use into account?
There was a problem hiding this comment.
We notify the event right before, and the finalization thread can't begin waiting until we release the lock, so nothing should be waiting on it at this point. I think there's a different bug here, though; memset isn't atomic, so the finalization thread might read the zero before it reads the "done" flag from the event.
I'll add a _PyEvent_Reset function that resets it with sequential ordering so this isn't a problem.
| // thread state was attached. | ||
| // To do this, we just use the memory address of a global variable and | ||
| // cast it to a PyThreadState *. | ||
| static const int NO_TSTATE_SENTINEL = 0; |
There was a problem hiding this comment.
Consider adding assert(tstate != NO_TSTATE_SENTINEL) to public functions that take a PyThreadState.
Also consider using an all-NULL PyThreadState struct, for debuggers.
Co-authored-by: Petr Viktorin <encukou@gmail.com>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
Hugo has graciously given me permission to backport this if we don't make the May 5th deadline, but let's try to get this done in time!
I will write a full tutorial and migration guide once this is merged; I want to first make sure that this lands before the beta freeze.